• Needs a test description 🔢 Applies elsewhere in the PR
• Needs an @covers annotation here.

same question from above: given that that test name itself forms the test description, and that the vast majority of existing tests rely on this, do we really need a duplicated description in every test?

@hellofromtonya Correct me if wrong, but should assertions only be performed inside the test methods / closures within test methods?

so this would be a good place to examine, as this is a fairly novel kind of test file. I have explored a few situations. the point here is to actively warn someone who wants to add additional support to the HTML parser class.

given that we have hundreds of tests that all do the same thing, I figured a minor abstraction here would be valuable to have, despite generally agreeing that test abstractions are things to be wary of.

if there's a way to ensure this won't be called outside of a test method that works; this was my intent on not naming it with the test_ prefix.

alternatively we could create a new assertion, which is essentially what this does, so I will look into that

the more I look into this the more I'd rather special-case this from the linting rules than create a new test assertion. creating a new assertion is going to pull in more complexity and make things more convoluted than they already are to debug and understand. having this single function, while different than in other cases, at least is self-contained and straightforward.

so I'd rather we do whatever we need to do to make sure this doesn't get abstracted into something much greater in scope than is required and allow this unique class to be unique.

more important than enforcing this styling rule, I'd love some feedback on automating the support rules, to ensure that nobody unintentionally partially adds support for HTML tags that require additional support. for example, we don't want someone to simply add a new BUTTON clause to the big switch statement in the html processor. if they do that, they will subvert other rules that have to be added to the adoption agency algorithm.

Noticed the * > format through a lot of the inline comments in the PR. This commenting style only appears to exist within the HTML API, and should likely be removed for consistency with the rest of Core.

These are quotes from the HTML specification and are important for associating the logic with the spec document, for being able to quickly find where in the spec this particular code is providing its functionality.

How should docblocks represent quotes? I will update as long as we can retain the clarity that they are quotes and as long as we don't reword them, since that would break the ability to link the spec with the code.

Of note, not every part of the spec we reference has an associated link, unfortunately, so these are incredibly valuable for quickly cross-referencing between the two sides.

Ah I understand now. In that case, I'd suggest we either keep the >, or remove them and start the quote with As stated by the HTML specification:. I don't have a preference there, so if you want to keep it as-is, that works for me!

I'm open to continue exploring ways to present these, but I think the > is succinct and helpful. Given how often we want to quote the spec I think As stated by the HTML specification: could get a bit distracting.

Where possible we have a link to the spec. Unfortunately in some of these places there are large sections that we split up in the logic, where each subsection doesn't have a unique URL.

Gutenberg mini-documents instead of Markdown docblocks? Could eliminate all the hassle over ordering and styles.

(Recurring) nit -- could do negative offset instead of subtraction:

Not sure it's that much nicer 🤷‍♂️

hm. maybe if others chime in. I'm confused more by negative positions, particularly when they also include math. I with the function would reject negative offsets so that subtraction and negative offsets couldn't overlap.

I think a negative offset with - $position - 1 is harder to read. What about 1 - $position? It would avoid the call to $this->count(), and while it's personally easier for me to read, I'm not sure if it would be as clear to others.

1 - $position is even less clear to me. I'm not worried about $this->count() as a call. although in this case it's a function call, we could decide if we measure that it's a problem to switch it to count( $this->stack ) which is essentially the same as a normal variable lookup.

I've rearranged this a bit to try and help. the loop index is now called $position_from_end to better indicate that it's not the normal array index. we're also using a helper variable $position_from_start to give a better name to the computation.

What's the $termination_list arg for? Are we missing some code (or a TODO note)?

might need to copy a comment from another function. it's unsupported by in the spec, and will eventually be filled out. thanks for noticing.

Not sure I've totally grasped this, but shouldn't this be something like

Good guess; it's confusing. We want to check if a given element, for example, a P, is in BUTTON scope. The BUTTON part is implied in the linked spec to define the set of termination elements, so BUTTON would actually end up in the second parameter, the array.

As it stands, it's not possible for the processor to encounter this because if it encounters a BUTTON it currently bails.

This odd tension between wanting something that will eventually support the full spec and something that can be reviewed leads to these oddities, and to things like the tests that ensure nobody adds BUTTON support without adjusting the corresponding functions.

Think it makes sense to add a PHPDoc @TODO or something of that sort, or would that also become unwieldy quickly due to the nature of these classes?

this is one of the things I've been trying to explore here. people don't seem to like @TODO (I personally find them useful in cases like this), I have tried ### HTML Support segments in docblocks, but those also don't seem great. this is just a small note attempting to indicate why the thing isn't wrong even though it appears wrong.

as you are pointing out, the wording is lacking, but that's a different issue IMO, something I will clean up

It probably doesn't matter much, but to aid debugging, we could use a naming convention that includes the tag name? Something like

$tag_name = $this->get_tag;
$i = 0;
do {
	$bookmark_name = $tag_name . '-' . $i++;
} while ( $this->has_bookmark( $bookmark_name ) );

Ah, oops: https://github.com/WordPress/wordpress-develop/pull/4519/files#diff-f822c7309a0a11d3993b378b85ef4a60588af0afac714f7e1590130aa4e300e5R834-R835 😅

we could add the tag name, but there's a tradeoff: when I've been debugging the number name is clear and terse while longer names take up more space, and if you have a slide of 20 DIVs the tag name isn't that helpful.

having a number was surprisingly helpful because the index corresponds to the tag in the sequence they appear.

what is your "oops" comment and link? is it the recommendation against programmatic names? that's not to warn against creating tag names in the bookmark name, but to warn against the very thing we're doing anyway with numbers.

I've taken the stance here that in this very controlled environment there's not a practical way to avoid doing that if we want to parse HTML without creating a second bookmarking system (which was explored). it's more like, "avoid doing this because it carries these risks. if you are aware of the risks and know what you are doing, fine."

🤐

A thing to keep in mind when coming across these is that they are direct quotes from the HTML spec. There are some good ones (look for An end tag whose tag name is "sarcasm")

$this->current_token seems to be internal state that's only ever set here (in step()) for subsequent usage in step_in_body() (which is private, and only called from here); plus it is read in seek() to determine direction.

Any chance we can avoid this? Getting all the bookkeeping right for $lexical_updates and $classname_updates in the Tag Processor was surprisingly complex (even though the concepts seemed deceptively simple), so I'd love if we could avoid introducing this piece of internal state here (and make more explicit who is going to be the consumer of current_token). For step_in_body() (and its prospective friends for other contexts), it seems easy enough to pass $current_token as an arg. Can we also find some sort of workaround for seek()?

this is a tough one, and I'm not sure I entirely follow the association with $lexical_updates and $classname_updates

I've always had a version of the current token because so much of the parsing algorithm depends on properties of the current element/node/token. it has made sense to make it globally available within the processor.

of note, it only gets set in the one place that all scanning takes place (everything flows through $this->step()) and ultimately it's just a bookmark.

I'd like to better understand the ways this could get out of sync. maybe it does when seeking backwards? I will try and think up a test to confirm that.

in f67e051 I added a new test which I actually though would break, but it seems to be fine. I don't have solid confidence that it's not still possible to break though.

Smart 👍

🤯

maybe this deserves a comment. I intentionally want to push in the tests situations that are deeper than specified in some rules relating to adoption and reconstruction, even if they don't relate specifically to each tag in the test.

with all the tests otherwise being small I figured we might miss certain things on a real document if we don't push the stack depth.

added a comment in 639a01f